cnf ran ptp: refactor OC 2 port 80963#1298
cnf ran ptp: refactor OC 2 port 80963#1298klaskosk wants to merge 1 commit intorh-ecosystem-edge:mainfrom
Conversation
📝 WalkthroughWalkthroughThe PTP active-port failover test has been refactored to replace event-consumer-based FREERUN validation with interface-role metric verification (FAULTY/FOLLOWER states). The restoration process now waits for OC 2-port roles to stabilize and shortens the required stable duration from 10s to 5s. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
This commit refactors the OC 2 port test 80963 to avoid a consistent failure we see in CI. The failover process is not instant, and may bounce into FREERUN briefly during the sync on the passive port. These are an artifact of how the feature works and not product bugs, so this commit updates the test to be less strict about these. Assisted-by: Cursor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
tests/cnf/ran/ptp/tests/ptp-oc-2-port.go (2)
435-452: Function name/comment overstate whatEventually…Should(Succeed())guarantees.The name
waitForOc2PortActivePassiveand comment "waits for OC 2-port roles to stabilize" imply sustained stability, butEventually(...).Should(Succeed())returns as soon asOc2PortDetermineActivePassiveInterfacessucceeds once — it does not verify the roles remain consistent across subsequent polls. In practice the downstreamAssertWithStableDurationonClockStateLockedcompensates, so this isn't a correctness bug, but consider either:
- Renaming/re-commenting to reflect "waits for roles to be determinable", or
- Requiring N consecutive successful determinations (e.g., same active/passive pair observed ≥2–3 times) to actually enforce stability.
✏️ Minimal doc-only tweak
-// waitForOc2PortActivePassive waits for OC 2-port roles to stabilize. +// waitForOc2PortActivePassive waits until OC 2-port active/passive roles can be +// successfully determined from interface-role metrics on the given node. func waitForOc2PortActivePassive(🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/cnf/ran/ptp/tests/ptp-oc-2-port.go` around lines 435 - 452, The function waitForOc2PortActivePassive currently returns as soon as Oc2PortDetermineActivePassiveInterfaces succeeds once, which overstates "stabilize"; either rename/comment the function to indicate it "waits for roles to be determinable" or change the implementation to require N consecutive identical determinations (e.g., observe the same active/passive pair from Oc2PortDetermineActivePassiveInterfaces for 2–3 consecutive polls) before succeeding; to implement the latter, call Oc2PortDetermineActivePassiveInterfaces repeatedly inside the Eventually loop, capture/compare the active/passive result (or relevant identifier) across iterations, count consecutive identical outcomes, and only return nil (success) when the consecutive count reaches N, otherwise reset the count on mismatch or error.
135-136: Tighten the failure message on the clock-class assertion."Relock failed" is copy-pasted from the clock-state check and is a bit misleading here — this assertion is about the grandmaster clock class, not process relock. A small wording tweak will make triage logs clearer.
✏️ Proposed wording
Expect(err).ToNot(HaveOccurred(), - "Relock failed: clock class did not return to 6 within %s", 90*time.Second) + "Convergence failed: ptp4l clock class did not return to 6 within %s", 90*time.Second)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/cnf/ran/ptp/tests/ptp-oc-2-port.go` around lines 135 - 136, The failure message for the clock-class assertion is misleadingly using "Relock failed"; update the Expect(err).ToNot(HaveOccurred(...) call in the test (the assertion that checks grandmaster clock class returning to 6) to use a clearer message such as "Grandmaster clock class did not return to 6 within %s" (preserving the 90*time.Second timeout placeholder) so logs reflect the actual check being performed.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@tests/cnf/ran/ptp/tests/ptp-oc-2-port.go`:
- Around line 435-452: The function waitForOc2PortActivePassive currently
returns as soon as Oc2PortDetermineActivePassiveInterfaces succeeds once, which
overstates "stabilize"; either rename/comment the function to indicate it "waits
for roles to be determinable" or change the implementation to require N
consecutive identical determinations (e.g., observe the same active/passive pair
from Oc2PortDetermineActivePassiveInterfaces for 2–3 consecutive polls) before
succeeding; to implement the latter, call
Oc2PortDetermineActivePassiveInterfaces repeatedly inside the Eventually loop,
capture/compare the active/passive result (or relevant identifier) across
iterations, count consecutive identical outcomes, and only return nil (success)
when the consecutive count reaches N, otherwise reset the count on mismatch or
error.
- Around line 135-136: The failure message for the clock-class assertion is
misleadingly using "Relock failed"; update the
Expect(err).ToNot(HaveOccurred(...) call in the test (the assertion that checks
grandmaster clock class returning to 6) to use a clearer message such as
"Grandmaster clock class did not return to 6 within %s" (preserving the
90*time.Second timeout placeholder) so logs reflect the actual check being
performed.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 6a896424-0478-4f7b-9e97-02289c8be06a
📒 Files selected for processing (1)
tests/cnf/ran/ptp/tests/ptp-oc-2-port.go
This commit refactors the OC 2 port test 80963 to avoid a consistent failure we see in CI. The failover process is not instant, and may bounce into FREERUN briefly during the sync on the passive port. These are an artifact of how the feature works and not product bugs, so this commit updates the test to be less strict about these.
Assisted-by: Cursor
Summary by CodeRabbit